Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

status: FromError: return entire error message text for wrapped errors #6150

Merged
merged 1 commit into from
Mar 27, 2023

Conversation

dfawley
Copy link
Member

@dfawley dfawley commented Mar 24, 2023

Many users would prefer to see the entire error message along with the wrapped status's code & details, so that data is not "lost". This change implements that preference.

I'm not convinced this is "correct" or "better", but it may be, and I wanted to discuss.

cc @psyhatter

RELEASE NOTES:

  • status: FromError: return entire error message text for wrapped errors

Comment on lines +82 to 84
// err.Error() text and not just the wrapped status.
//
// - If err is nil, a Status is returned with codes.OK and no message.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: can we explicitly specify it returns true in these two cases, and not use the fact that this is the if on the else of the third case which returns false

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO this is such a common pattern that the extra sentences to document "ok" wouldn't be worth worth the clutter they would create. E.g. this is the documentation of context.Deadline():

	// Deadline returns the time when work done on behalf of this context
	// should be canceled. Deadline returns ok==false when no deadline is
	// set. Successive calls to Deadline return the same results.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok fair enough

@dfawley
Copy link
Member Author

dfawley commented Mar 24, 2023

[I should link the original PR #6031 that added wrapping support from this PR, too.]

@dfawley dfawley added the Type: Feature New features or improvements in behavior label Mar 24, 2023
@dfawley dfawley added this to the 1.55 Release milestone Mar 24, 2023
@psyhatter
Copy link
Contributor

psyhatter commented Mar 25, 2023

@dfawley This would indeed be helpful =)

Since the response from the server is formed using this function, in this example the response from the server will be more friendly like this:
getting sender: rpc error: code = NotFound desc = user not found

Instead of like now:
rpc error: code = NotFound desc = user not found

The only thing is that some error parsers do not expect such changes

For example, now the grpc client in jet brains ide returns the following message:
com.intellij.grpc.requests.RejectedRPCException: Unknown error occurred duing request execution: "NOT_FOUND: user not found"

After these changes will be something like this:
com.intellij.grpc.requests.RejectedRPCException: Unknown error occurred duing request execution: "NOT_FOUND: getting sender: rpc error: code = NotFound desc = user not found"

Copy link
Contributor

@zasweq zasweq left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@zasweq zasweq assigned dfawley and unassigned zasweq Mar 27, 2023
@dfawley
Copy link
Member Author

dfawley commented Mar 27, 2023

The only thing is that some error parsers do not expect such changes

I'm fairly sure error strings are not considered part of the API surface; I know the Go standard libraries have changed their error strings at least once or twice. I think this is a good change - also remember that no released version of gRPC works with wrapped status errors anyway (your PR went in after the latest release), so the only real behavior change is that wrapped status errors now unwrap.

@dfawley dfawley merged commit a357baf into grpc:master Mar 27, 2023
1 check passed
@dfawley dfawley deleted the status_wrap branch March 27, 2023 23:01
atollena added a commit to atollena/google-cloud-go that referenced this pull request Jul 3, 2023
gRPC v1.55.0 that this project depends on came with a change in semantic for status.FromError such
that the gRPC status of wrapped errors is returned. The implementation of GRPCStatus in
internal/retry code was doing just that. Removing GRPCStatus entirely makes the code much easier to
understand: at the moment, status.FromError calls GRPCStatus which in turns calls status.FromError
and it's not immediately obviuos why this can't result in infinite recursion.

A previous change I made to this code made sure this method never returns
Status.OK (googleapis#8128), but I failed to realize that
since this project depends on gRPC 1.55 that already handles wrapped errors in status.FromError, we
can simply remove the implementation of `GRPCStatus` and let gRPC status.FromError handle
unwrapping.

Note that I barely had to change the tests, but there *IS* a slight change in behavior: the message
of the wrapping error is included in the gRPC error. I think it's fine, bet let me know if you think
otherwise (for the same reasons discussed in grpc/grpc-go#6150).
atollena added a commit to atollena/google-cloud-go that referenced this pull request Jul 3, 2023
gRPC v1.55.0 with a change in semantic for status.FromError such that the gRPC status of wrapped
errors is returned when defined. The implementation of GRPCStatus in internal/retry code was doing
just that. Removing GRPCStatus entirely makes the code much easier to understand: at the moment,
status.FromError calls GRPCStatus which in turns calls status.FromError and it's not immediately
obvious why this cannot result in infinite recursion.

A previous change I made to this code made sure this method never returns
status Code.OK (googleapis#8128), but I failed to realize that
since this project depends on gRPC 1.55 that already handles wrapped errors in status.FromError, we
can simply remove the implementation of `GRPCStatus` and let gRPC status.FromError handle
unwrapping.

Note that I barely had to change the tests, but there *IS* a slight change in behavior: the message
of the wrapping error is included in the gRPC error. I think it's fine, bet let me know if you think
otherwise (for the same reasons discussed in grpc/grpc-go#6150).
atollena added a commit to atollena/google-cloud-go that referenced this pull request Jul 3, 2023
gRPC v1.55.0 with a change in semantic for status.FromError such that the gRPC status of wrapped
errors is returned when defined. The implementation of GRPCStatus in internal/retry code was doing
just that. Removing GRPCStatus entirely makes the code much easier to understand: at the moment,
status.FromError calls GRPCStatus which in turns calls status.FromError and it's not immediately
obvious why this cannot result in infinite recursion.

A previous change I made to this code made sure this method never returns
status Code.OK (googleapis#8128), but I failed to realize that
since this project depends on gRPC 1.55 that already handles wrapped errors in status.FromError, we
can simply remove the implementation of `GRPCStatus` and let gRPC status.FromError handle
unwrapping.

Note that I barely had to change the tests, but there *IS* a slight change in behavior: the message
of the wrapping error is included in the gRPC error. I think it's fine, bet let me know if you think
otherwise (for the same reasons discussed in grpc/grpc-go#6150).
atollena added a commit to atollena/google-cloud-go that referenced this pull request Jul 3, 2023
gRPC v1.55.0 with a change in semantic for status.FromError such that the gRPC status of wrapped
errors is returned when defined. The implementation of GRPCStatus in internal/retry code was doing
just that. Removing GRPCStatus entirely makes the code much easier to understand: at the moment,
status.FromError calls GRPCStatus which in turns calls status.FromError and it's not immediately
obvious why this cannot result in infinite recursion.

A previous change I made to this code made sure this method never returns
status Code.OK (googleapis#8128), but I failed to realize that
since this project depends on gRPC 1.55 that already handles wrapped errors in status.FromError, we
can simply remove the implementation of `GRPCStatus` and let gRPC status.FromError handle
unwrapping.

Note that I barely had to change the tests, but there *IS* a slight change in behavior: the message
of the wrapping error is included in the gRPC error. I think it's fine, bet let me know if you think
otherwise (for the same reasons discussed in grpc/grpc-go#6150).
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Type: Feature New features or improvements in behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants